Add embedded surface policies for Cherrypick file editor#2
Conversation
Summary By MatterAI
🔄 What ChangedImplemented a reactive event system for the editor's selection menu. Added 🔍 Impact of the ChangeEnsures UI consistency between the editor's internal state and the toolbar. When the selection menu is toggled (e.g., in the Cherrypick file editor), the toolbar items now correctly hide or reposition themselves immediately, improving the reliability of embedded surface policies. 📁 Total Files ChangedClick to Expand
🧪 Test Added/RecommendedAdded
Recommended
🔒 Security Vulnerabilities
Objective
Solution
Testing
Self-Review Checklist:
Showcase
Release Notes:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThree new policy structs control which context-menu action groups (language, agent, preview, terminal, git, workspace-folder, search, compare) appear in Editor, ProjectPanel, and Pane context menus. Each policy is stored on its component, initialized to full, and enforced throughout menu builders. A new EditorSelectionControls toolbar component centralizes selection-menu UI. File finder toggle logic is extracted into a helper function. ProjectPanel gains a reveal_abs_path method and Workspace exposes a modal_layer accessor. Quick action bar is refactored to use the new selection controls component. ChangesPolicies, Selection Controls, and UI Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoAdd policy-driven context menus for embedded editor and project panel Description
Diagram
High-Level Assessment
Files changed (5)
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces context menu policies (EditorSurfacePolicy, ProjectPanelContextMenuPolicy, and PaneContextMenuPolicy) to conditionally show or hide actions in the editor, project panel, and pane context menus. It also adds a helper method reveal_abs_path to ProjectPanel to reveal a project entry at an absolute path. The feedback suggests simplifying a nested if/else block in pane.rs and renaming abbreviated variables like abs_path and ext to full words (absolute_path, path, and extension) to adhere to the repository style guide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let parent_abs_path = | ||
| if pane.read(cx).context_menu_policy.show_terminal_actions { | ||
| entry_abs_path | ||
| .as_deref() | ||
| .and_then(|abs_path| Some(abs_path.parent()?.to_path_buf())) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
The nested if/else block can be simplified using bool::then and .flatten(). Additionally, the closure parameter abs_path should be renamed to path to avoid abbreviations, adhering to the repository style guide.
let parent_abs_path = pane
.read(cx)
.context_menu_policy
.show_terminal_actions
.then(|| {
entry_abs_path
.as_deref()
.and_then(|path| path.parent().map(|p| p.to_path_buf()))
})
.flatten();References
- Use full words for variable names (no abbreviations like 'q' for 'queue'). (link)
| pub fn reveal_abs_path( | ||
| &mut self, | ||
| project: Entity<Project>, | ||
| abs_path: &Path, | ||
| window: &mut Window, | ||
| cx: &mut Context<Self>, | ||
| ) -> Result<()> { | ||
| let Some(project_path) = project | ||
| .read(cx) | ||
| .project_path_for_absolute_path(abs_path, cx) | ||
| else { | ||
| anyhow::bail!("path is not part of the project"); | ||
| }; |
There was a problem hiding this comment.
The parameter abs_path should be renamed to absolute_path to avoid abbreviations, adhering to the repository style guide.
pub fn reveal_abs_path(
&mut self,
project: Entity<Project>,
absolute_path: &Path,
window: &mut Window,
cx: &mut Context<Self>,
) -> Result<()> {
let Some(project_path) = project
.read(cx)
.project_path_for_absolute_path(absolute_path, cx)
else {
anyhow::bail!("path is not part of the project");
};References
- Use full words for variable names (no abbreviations like 'q' for 'queue'). (link)
| .is_some_and(|file| { | ||
| std::path::Path::new(file.file_name(cx)) | ||
| .extension() | ||
| .is_some_and(|ext| ext.eq_ignore_ascii_case("svg")) |
There was a problem hiding this comment.
The closure parameter ext should be renamed to extension to avoid abbreviations, adhering to the repository style guide.
| .is_some_and(|ext| ext.eq_ignore_ascii_case("svg")) | |
| .is_some_and(|extension| extension.eq_ignore_ascii_case("svg")) |
References
- Use full words for variable names (no abbreviations like 'q' for 'queue'). (link)
|
✅ Reviewed the changes: Clean, well-structured PR introducing surface-specific context menu policies across Editor, ProjectPanel, and Pane. Reviewed crates/editor/src/editor.rs: no issues found. Reviewed crates/editor/src/mouse_context_menu.rs: no issues found. Reviewed crates/project_panel/src/project_panel.rs: no issues found. Reviewed crates/workspace/src/pane.rs: no issues found. |
There was a problem hiding this comment.
Pull request overview
Adds configurable “policy” flags to control which context-menu actions are available across editor surfaces, panes, and the project panel, plus a new helper to reliably reveal a path in the project panel even during early-mount timing.
Changes:
- Introduced policy structs + setters to enable/disable groups of context-menu actions in the editor, pane tab menu, and project panel.
- Updated editor and project panel context menus to conditionally include terminal/git/preview/language/agent/compare/search/workspace actions based on policy.
- Added
ProjectPanel::reveal_abs_pathwith state seeding to avoid a race where reveal/expand could silently no-op shortly after mount.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/workspace/src/pane.rs | Add PaneContextMenuPolicy + setter and gate “Open in Terminal” tab action accordingly. |
| crates/project_panel/src/project_panel.rs | Add ProjectPanelContextMenuPolicy + setter, gate context-menu sections, and add reveal_abs_path to fix early reveal expansion race. |
| crates/editor/src/mouse_context_menu.rs | Respect EditorSurfacePolicy flags when building the editor mouse context menu. |
| crates/editor/src/items.rs | Hide preview-related item actions when preview actions are disabled by policy. |
| crates/editor/src/editor.rs | Add EditorSurfacePolicy + setter to configure which action groups are available on an editor surface. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub const fn file_editor() -> Self { | ||
| Self { | ||
| show_language_actions: false, | ||
| show_agent_actions: false, | ||
| show_preview_actions: false, | ||
| show_terminal_actions: false, | ||
| show_git_actions: false, | ||
| } | ||
| } |
| pub const fn file_editor() -> Self { | ||
| Self { | ||
| show_terminal_actions: false, | ||
| } | ||
| } |
| // `reveal_entry` -> `expand_entry` only records expanded ancestors when the | ||
| // worktree already has an `expanded_dir_ids` entry. That map is seeded | ||
| // asynchronously by `update_visible_entries` (its vacant branch inserts the | ||
| // root entry), so a reveal issued right after mount can beat it | ||
| // and `expand_entry`'s `get_mut` returns `None` — it silently no-ops and the | ||
| // tree never expands to the target file. Seed the worktree root here if | ||
| // it's still vacant (same value `update_visible_entries` would insert) so | ||
| // `expand_entry` writes the ancestors into `self.state` synchronously; every | ||
| // later `update_visible_entries` derives from that state and preserves them. | ||
| let worktree_id = project_path.worktree_id; | ||
| if !self.state.expanded_dir_ids.contains_key(&worktree_id) | ||
| && let Some(root_id) = project | ||
| .read(cx) | ||
| .worktree_for_id(worktree_id, cx) | ||
| .and_then(|worktree| worktree.read(cx).root_entry().map(|entry| entry.id)) | ||
| { | ||
| self.state | ||
| .expanded_dir_ids | ||
| .insert(worktree_id, vec![root_id]); | ||
| } |
Code Review by Qodo
1.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/editor/src/editor.rs`:
- Line 1120: The `surface_policy` field added to the `Editor` struct is not
being copied when the editor is cloned. To fix this, locate the `Clone`
implementation or clone method for the `Editor` struct and ensure that the
`surface_policy` field is explicitly copied along with all other fields when
creating the cloned instance. This will prevent clones from silently falling
back to the default surface policy value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea7afb31-c458-4730-8fa2-fcee55df620a
📒 Files selected for processing (5)
crates/editor/src/editor.rscrates/editor/src/items.rscrates/editor/src/mouse_context_menu.rscrates/project_panel/src/project_panel.rscrates/workspace/src/pane.rs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/project_panel/src/project_panel.rs">
<violation number="1" location="crates/project_panel/src/project_panel.rs:3958">
P3: New context-menu policy wiring is unused; added policy branches are unreachable from current code paths.</violation>
</file>
<file name="crates/editor/src/editor.rs">
<violation number="1" location="crates/editor/src/editor.rs:3006">
P2: `set_surface_policy` is unused, so `surface_policy` stays default and non-default surface behavior is currently unreachable.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| self.in_project_search = in_project_search; | ||
| } | ||
|
|
||
| pub fn set_surface_policy(&mut self, policy: EditorSurfacePolicy, cx: &mut Context<Self>) { |
There was a problem hiding this comment.
P2: set_surface_policy is unused, so surface_policy stays default and non-default surface behavior is currently unreachable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/editor/src/editor.rs, line 3006:
<comment>`set_surface_policy` is unused, so `surface_policy` stays default and non-default surface behavior is currently unreachable.</comment>
<file context>
@@ -2964,6 +3003,14 @@ impl Editor {
self.in_project_search = in_project_search;
}
+ pub fn set_surface_policy(&mut self, policy: EditorSurfacePolicy, cx: &mut Context<Self>) {
+ if self.surface_policy == policy {
+ return;
</file context>
| }) | ||
| } | ||
|
|
||
| pub fn set_context_menu_policy( |
There was a problem hiding this comment.
P3: New context-menu policy wiring is unused; added policy branches are unreachable from current code paths.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/project_panel/src/project_panel.rs, line 3958:
<comment>New context-menu policy wiring is unused; added policy branches are unreachable from current code paths.</comment>
<file context>
@@ -3905,6 +3955,18 @@ impl ProjectPanel {
})
}
+ pub fn set_context_menu_policy(
+ &mut self,
+ policy: ProjectPanelContextMenuPolicy,
</file context>
soyboyscout
changed the title
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/project_panel/src/project_panel.rs">
<violation number="1" location="crates/project_panel/src/project_panel.rs:3958">
P3: New context-menu policy wiring is unused; added policy branches are unreachable from current code paths.</violation>
</file>
<file name="crates/editor/src/editor.rs">
<violation number="1" location="crates/editor/src/editor.rs:3006">
P2: `set_surface_policy` is unused, so `surface_policy` stays default and non-default surface behavior is currently unreachable.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/editor/src/selection_controls.rs`:
- Around line 125-130: The settings_subscription at line 125 only observes
SettingsStore changes but does not track changes to the editor-local
show_selection_menu state that affects selection_menu_enabled. Add a separate
subscription (in addition to the existing SettingsStore observation) that
observes the active editor state to detect when show_selection_menu changes, and
emit the ToolbarItemEvent::ChangeLocation event with the updated
toolbar_item_location in response to these editor state changes to keep toolbar
visibility in sync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9fa56813-aa15-44b8-b04e-9a50cd13e0cd
📒 Files selected for processing (10)
crates/editor/src/editor.rscrates/editor/src/editor_tests.rscrates/editor/src/mouse_context_menu.rscrates/editor/src/selection_controls.rscrates/file_finder/src/file_finder.rscrates/project_panel/src/project_panel.rscrates/project_panel/src/project_panel_tests.rscrates/workspace/src/pane.rscrates/workspace/src/workspace.rscrates/zed/src/zed/quick_action_bar.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/editor/src/mouse_context_menu.rs
- crates/workspace/src/pane.rs
Objective
Cherrypick embeds Zed's project panel, pane, and editor as a restricted file browser/editor surface. The embedded surface needs curated context menus because Cherrypick owns workspace-level actions such as terminal launch, git workflows, agent actions, and folder management.
This is not intended to add a standalone Zed product setting. Normal Zed behavior remains the default through the
full()policy presets.Solution
EditorSurfacePolicyProjectPanelContextMenuPolicyPaneContextMenuPolicyfull()presets for standard Zed behavior andembedded()presets for Cherrypick's restricted file browser/editor surface.ProjectPanel::reveal_abs_pathuse the panel's canonicalself.projectinternally, avoiding mixed-project state from external callers.Editor::clone.Testing
Zed fork checks:
CARGO_TARGET_DIR=/tmp/cherrypick-zed-feat-code-editor-target cargo +1.95.0 test -p editor test_cloneCARGO_TARGET_DIR=/tmp/cherrypick-zed-feat-code-editor-target cargo +1.95.0 test -p editor test_embedded_surface_policy_hides_host_owned_actionsCARGO_TARGET_DIR=/tmp/cherrypick-zed-feat-code-editor-target cargo +1.95.0 test -p project_panel test_embedded_project_panel_context_menu_policy_hides_host_owned_actionsCARGO_TARGET_DIR=/tmp/cherrypick-zed-feat-code-editor-target cargo +1.95.0 test -p project_panel test_reveal_abs_path_seeds_missing_expanded_stateCARGO_TARGET_DIR=/tmp/cherrypick-zed-feat-code-editor-target cargo +1.95.0 test -p workspace test_embedded_pane_context_menu_policy_hides_host_owned_actionsgit diff --checkCherrypick consumer checks:
CARGO_TARGET_DIR=/tmp/cherrypick-feat-code-editor-pr2-target cargo +1.95.0 test -p cherrypick-ui zed_files_host --libCARGO_TARGET_DIR=/tmp/cherrypick-feat-code-editor-pr2-target cargo +1.95.0 build --release -p cherrypick-ui --bin cherrypick/tmp/cherrypick-feat-code-editor-pr2-target/release/cherrypick; startup log reportsMCP bridge listening.Self-Review Checklist
Reviewer Context
The policy setters are consumed from Cherrypick's
zed_files_host.rs, outside this Zed fork repository. Review comments that report the setters as unused in standalone Zed are expected from a Zed-only diff; the Cherrypick consumer build above verifies the integration point.Release Notes:
Summary by CodeRabbit